Skip to content

fix: Unify Actor context manager with init & exit methods #600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Oct 15, 2025
Merged

fix: Unify Actor context manager with init & exit methods #600

merged 9 commits into from
Oct 15, 2025

Conversation

@vdusek
Copy link
Contributor

@vdusek vdusek commented Sep 19, 2025 •
edited
Loading

Description

To align the behavior between the two possible approaches of using an Actor - as a context manager or by explicitly calling init/exit/fail - the following changes were implemented:

  • The Actor's arguments (exit_code, status_message, event_listeners_timeout, cleanup_timeout) were added to the Actor.__init__ as well.
  • exit_code and status_message are now public properties with setters, allowing them to be updated within a context manager block in response to error conditions.
  • The previous approach of calling __aenter__ from init and __aexit__ from exit was reversed, ensuring fewer inconsistencies for future changes (since __aenter__ and __aexit__ have a fixed number of arguments).

Other changes:

  • Related docstrings were updated.
  • The ordering of methods in the Actor was adjusted (see ruff issue #2425).
  • The interface of init, exit, and fail remains unchanged.
  • Many more tests have been added.
  • The documentation page about the Actor lifecycle has been completely rewritten.

Issues

Testing

  • The current test set cover the changes.

@vdusek vdusek self-assigned this Sep 19, 2025
@vdusek vdusek added the t-tooling Issues with this label are in the ownership of the tooling team. label Sep 19, 2025
@github-actions github-actions bot added this to the 123rd sprint - Tooling team milestone Sep 19, 2025
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Sep 19, 2025
@vdusek vdusek force-pushed the init-exit branch 2 times, most recently from fcec3b9 to 3dacd65 Compare September 19, 2025 16:05
@Pijukatel
Copy link
Contributor

Pijukatel commented Sep 22, 2025

Am not sure about this. What about this scenario?

...
async with Actor(...):
    await Actor.fail(exit_code=1, status_message='This is a test message')

This used to work before, but now it will try to call __aexit__ twice

@vdusek
Copy link
Contributor Author

vdusek commented Sep 22, 2025

IMO that scenario is a misuse. You've got 2 approaches and you should choose one of them. Not combining them.

@B4nan
Copy link
Member

B4nan commented Sep 22, 2025 •
edited
Loading

People can use Actor.fail (as well as Actor.exit) as part of their code, even in the request handler, so I wouldn't call that a misuse really. They might want to quit prematurely on purpose, the method is not always supposed to be at the very end of the main script. In JS, it does process.exit so it quits the process completely, so you won't fall into this, the top level Actor.exit wouldn't be called if that happens.

Can we keep a flag to know the exit method was already called, and skip the duplicated logic that would be otherwise produced by the context manager?

@Pijukatel
Copy link
Contributor

Pijukatel commented Sep 25, 2025

I added one test. We can decide whether it is an allowed use case that should be supported or a misuse.
a03b3f1

janbuchar
janbuchar reviewed Oct 8, 2025
View reviewed changes
Copy link
Contributor

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Judging by https://github.com/apify/apify-sdk-python/pull/600/files#diff-807ba3710b8cdf01f23374c98d85a91075d4b602c8c4a3b46ddb64696b16987d I suppose the point raised by @Pijukatel has been resolved?

@vdusek
Copy link
Contributor Author

vdusek commented Oct 9, 2025

If you mean this:

Am not sure about this. What about this scenario?

...
async with Actor(...):
    await Actor.fail(exit_code=1, status_message='This is a test message')

This used to work before, but now it will try to call aexit twice

Then yes, this is a valid scenario (we have the _is_exiting flag).

@vdusek vdusek changed the title refactor: Unify Actor context manager with init & exit methods fix: Unify Actor context manager with init & exit methods Oct 10, 2025
janbuchar
janbuchar reviewed Oct 10, 2025
View reviewed changes
@vdusek vdusek requested a review from janbuchar October 13, 2025 19:52
@vdusek
Copy link
Contributor Author

vdusek commented Oct 13, 2025

@janbuchar @Pijukatel could you guys take another look? Thanks.

janbuchar
janbuchar approved these changes Oct 14, 2025
View reviewed changes
Pijukatel
Pijukatel requested changes Oct 15, 2025
View reviewed changes
Copy link
Contributor

@Pijukatel Pijukatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actor.fail should jump out of the context, but it does not. The test I added earlier was removed, but it can be seen there:

Without this, the user will not be able to fail/exit Actor from request handlers.

async def test_actor_fail_prevents_further_execution(caplog: pytest.LogCaptureFixture) -> None:
    async with Actor:
        await Actor.fail(exit_code=2, exception=Exception('abc'), status_message='cde')
        raise RuntimeError('This should not trigger')
    assert caplog.records[-3].levelno == logging.ERROR  # type: ignore[unreachable]
    assert caplog.records[-3].msg == 'Actor failed with an exception'
    assert caplog.records[-3].exc_text == 'Exception: abc'
    assert caplog.records[-2].levelno == logging.INFO
    assert caplog.records[-2].msg == 'Exiting Actor'
    assert caplog.records[-1].levelno == logging.INFO
    assert caplog.records[-1].msg == '[Terminal status message]: cde'

@vdusek vdusek requested a review from Pijukatel October 15, 2025 11:59
@vdusek
Copy link
Contributor Author

vdusek commented Oct 15, 2025

@Pijukatel The test is back and fixes.

@vdusek vdusek merged commit 6b0d084 into master Oct 15, 2025
21 checks passed
@vdusek vdusek deleted the init-exit branch October 15, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@janbuchar janbuchar janbuchar approved these changes

@Pijukatel Pijukatel Pijukatel approved these changes

Assignees

@vdusek vdusek

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Milestone

125th sprint - Tooling team

Development

Successfully merging this pull request may close these issues.

Unify Actor's context manager with manual init/exit methods

5 participants

@vdusek @Pijukatel @B4nan @janbuchar